The logo goes on every clip, the outro plays whole, and a guest has a name - #148
The logo goes on every clip, the outro plays whole, and a guest has a name#148nmbrthirteen wants to merge 1 commit into
Conversation
… name Four things measured against a published short that was cut with this very renderer. None of them needed a new feature; all four were the renderer not doing what it was told. `--logo` was accepted, resolved from the asset store, and then dropped unless the caption style happened to be "branded". Two gates did it: a `logo_support` flag in the style config, and the fact that the logo was drawn inside the branded caption component. A logo belongs to the show, not to a caption style. Both gates are gone, the mark moved one level up into CaptionedClip at exactly its old position, and a branded render is byte-identical, frame hash for frame hash. Subtle captions faded in and then cut, so every chunk boundary flickered: the outgoing line vanished on the frame the incoming one started at zero. They now ramp down inside their own window, which crossfades without moving a single caption timing. Skipped on chunks too short to hold full opacity. An intro or an outro was joined with 0.8 seconds of crossfade through black, which ate three quarters of a second of both. Six seconds of clip plus two of outro came out at 7.23. A designed bookend should be cut to; the fade is now a number, and it defaults to none. And a clip lifted out of an hour of conversation opens on a stranger, so there is a lower third: name, role, an accent underline, gone after three seconds. `--name-card`, `--name-card-sub`. Two regression tests, because this class of bug is silent: one asserts no caption style may gate the logo, the other that the name card reaches the renderer at all. The suite already caught one live mistake here — a parameter used in a body whose signature never got it, swallowed by the surrounding except.
📝 WalkthroughWalkthroughThe PR adds process options for speaker name cards and bookend fades. It forwards these settings through clip generation into Remotion, adds animated name-card and watermark components, removes style-based logo gating, and updates caption fade behavior. ChangesClip rendering enhancements
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_clip_generator.py (1)
99-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the complete name-card command contract.
The test does not pass or assert
secondsandaccent. A regression that drops either--name-card-secondsor--name-card-accentwill pass this test.Add both values to
name_card. Assert that each flag and serialized value reaches the command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_clip_generator.py` around lines 99 - 107, Update test_name_card_reaches_the_renderer to include representative seconds and accent values in name_card, then assert that --name-card-seconds and --name-card-accent and their serialized values are present in argv alongside the existing title and subtitle assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cli.py`:
- Around line 1067-1068: Update every interactive re-render generate_clip call
to pass the same config.get("name_card") and config.get("bookend_fade", 0.0)
values already used by the initial render, preserving overlay settings after
review edits.
- Around line 585-586: Update the --bookend-fade argument definition to use
default=None, preserving the existing args.bookend_fade is not None guard in the
configuration update so preset bookend_fade values are overridden only when the
CLI option is explicitly supplied.
In `@backend/services/clip_generator.py`:
- Around line 923-924: Update the ASS fallback path around the clip-generation
logic at lines 934-958 so a supplied name_card is rendered as an equivalent
lower-third overlay. Ensure this applies when use_ass_captions is enabled and
when Remotion falls back to ASS, including --fast; if ASS cannot support
name_card, reject the option combination before generating a clip rather than
silently omitting it.
In `@remotion/src/components/NameCard.tsx`:
- Around line 41-44: Update the rendering logic in NameCard around fadeInOut so
frames at or beyond seconds * fps return null, ensuring the card unmounts at its
configured duration. Also validate or clamp seconds so durations shorter than
the fade ramps cannot produce unsupported fade behavior.
---
Nitpick comments:
In `@tests/test_clip_generator.py`:
- Around line 99-107: Update test_name_card_reaches_the_renderer to include
representative seconds and accent values in name_card, then assert that
--name-card-seconds and --name-card-accent and their serialized values are
present in argv alongside the existing title and subtitle assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33715186-0115-435c-b030-4a696fb0f039
📒 Files selected for processing (12)
backend/cli.pybackend/config/caption_styles.pybackend/services/clip_generator.pyremotion/render.mjsremotion/src/CaptionedClip.tsxremotion/src/Root.tsxremotion/src/components/BrandedCaptions.tsxremotion/src/components/NameCard.tsxremotion/src/components/SubtleCaptions.tsxremotion/src/components/Watermark.tsxremotion/src/motion.tstests/test_clip_generator.py
💤 Files with no reviewable changes (2)
- backend/config/caption_styles.py
- remotion/src/components/BrandedCaptions.tsx
| if getattr(args, "bookend_fade", None) is not None: | ||
| config["bookend_fade"] = args.bookend_fade |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not overwrite the preset fade when the option is absent.
--bookend-fade defaults to 0.0. Therefore Line 585 always replaces a preset bookend_fade value with a hard cut.
Use default=None in the argument definition. Keep the existing is not None check so only an explicit CLI option overrides the preset.
Proposed fix
-proc.add_argument("--bookend-fade", dest="bookend_fade", type=float, default=0.0,
+proc.add_argument("--bookend-fade", dest="bookend_fade", type=float, default=None,
help="Seconds of crossfade into an intro or outro (default 0, a cut)")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cli.py` around lines 585 - 586, Update the --bookend-fade argument
definition to use default=None, preserving the existing args.bookend_fade is not
None guard in the configuration update so preset bookend_fade values are
overridden only when the CLI option is explicitly supplied.
| name_card=config.get("name_card"), | ||
| bookend_fade=config.get("bookend_fade", 0.0), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward overlay settings during re-renders.
The initial render receives name_card and bookend_fade. The interactive re-render at Lines 1270-1287 omits both arguments. After any review edit, the replacement clip loses the name card and reverts bookend joins to a hard cut.
Pass the same two configuration values to every re-render generate_clip call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cli.py` around lines 1067 - 1068, Update every interactive re-render
generate_clip call to pass the same config.get("name_card") and
config.get("bookend_fade", 0.0) values already used by the initial render,
preserving overlay settings after review edits.
| logo_path=logo_path or None, | ||
| name_card=name_card, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently drop name_card on the ASS render path.
name_card is sent only to Remotion. If use_ass_captions is enabled, or Remotion fails and ASS fallback is allowed, Lines 934-958 emit a clip without the requested lower third. --fast enables this path.
Render an equivalent name card in the fallback path. If that is not supported, reject this option combination instead of producing a clip that omits the requested overlay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/services/clip_generator.py` around lines 923 - 924, Update the ASS
fallback path around the clip-generation logic at lines 934-958 so a supplied
name_card is rendered as an equivalent lower-third overlay. Ensure this applies
when use_ass_captions is enabled and when Remotion falls back to ASS, including
--fast; if ASS cannot support name_card, reject the option combination before
generating a clip rather than silently omitting it.
| const opacity = fadeInOut({ | ||
| frame, fps, start: 0, end: seconds, inFrames: 8, outFrames: 10, | ||
| }); | ||
| if (opacity <= 0) return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unmount the card after its configured duration.
If seconds is shorter than the fade ramps, fadeInOut() returns the rising opacity only. After it reaches 1, this component remains visible for the rest of the clip instead of ending at seconds.
Check the current frame against seconds * fps before rendering. Also validate or clamp unsupported short durations.
Proposed fix
const opacity = fadeInOut({
frame, fps, start: 0, end: seconds, inFrames: 8, outFrames: 10,
});
- if (opacity <= 0) return null;
+ if (frame >= Math.round(seconds * fps) || opacity <= 0) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const opacity = fadeInOut({ | |
| frame, fps, start: 0, end: seconds, inFrames: 8, outFrames: 10, | |
| }); | |
| if (opacity <= 0) return null; | |
| const opacity = fadeInOut({ | |
| frame, fps, start: 0, end: seconds, inFrames: 8, outFrames: 10, | |
| }); | |
| if (frame >= Math.round(seconds * fps) || opacity <= 0) return null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@remotion/src/components/NameCard.tsx` around lines 41 - 44, Update the
rendering logic in NameCard around fadeInOut so frames at or beyond seconds *
fps return null, ensuring the card unmounts at its configured duration. Also
validate or clamp seconds so durations shorter than the fade ramps cannot
produce unsupported fade behavior.
1 participant
Four fixes, all measured against a published Deeptech Decoded short that was cut with this renderer. None needed a new feature; each was the renderer not doing what it was told.
--logowas dropped on three of four caption stylesTwo gates: a
logo_supportflag in the style config that onlybrandedset true, and the logo being drawn inside the branded caption component. So the flag was accepted, the asset resolved, and the watermark silently never drawn onhormozi,karaokeorsubtle.A logo belongs to the show, not to a caption style. Both gates are gone and the mark moved one level up into
CaptionedClip, at exactly its old position and size.A branded render is byte-identical — same frame hash before and after (
6bba026c…).Subtle captions faded in and then cut
Every chunk boundary flickered: the outgoing line vanished on the frame the incoming one started at zero. They ramp down inside their own window now, which crossfades without moving a single caption timing. Skipped on chunks too short to hold full opacity.
An intro or outro was joined through 0.8s of black
concat_outrodefaulted tocrossfade_duration=0.8, transition="fadeblack"andgenerate_clipnever overrode it, so it ate three quarters of a second of both sides. Measured: 6s of clip + 2s of outro came out at 7.23s.A designed bookend should be cut to. The fade is a number now (
--bookend-fade) and defaults to none — 8.03s for the same inputs.A clip opens on a stranger
New lower third: name, role, an accent underline, gone after three seconds.
--name-card "Jamie Gull: Founder & Solo GP" --name-card-sub "at Wave Function Ventures".Tests
Two regressions, because this class of bug is silent:
logo_supportgate (fails if a per-style opt-out returns)The existing suite already caught a live mistake in this branch: a parameter used in a body whose signature never got it, swallowed by the surrounding
except, which showed up only as two subprocess calls instead of four.test_ai_fallback's three CLI-discovery failures are pre-existing onmainand unrelated.Summary by CodeRabbit
New Features
Bug Fixes
Tests